Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature addition: Multi-Factor Authentication #401

Draft
wants to merge 32 commits into
base: develop
Choose a base branch
from
Draft

Conversation

chesspro13
Copy link

@chesspro13 chesspro13 commented Sep 7, 2024

Features added

TOTP (Time-based One-Time Password) with recovery codes
OpenID/SSO via Google (for now)

Documentation

Testing Instructions

TOTP

  1. Start Trilium Notes normally.
  2. Go to "Menu" -> "Options" -> "MFA"
  3. Click the "Generate TOTP Secret" button
  4. Copy the generated secret to your authentication app/extension
  5. Set an environment variable "TOTP_SECRET" as the generated secret. Environment variables can be set with a .env file in the root directory, by defining them in the command line, or with a docker container.
    # .env in the project root directory
    TOTP_ENABLED="true"
    TOTP_SECRET="secret"
    # Terminal/CLI
    export TOTP_ENABLED="true"
    export TOTP_SECRET="secret"
    # Docker
    docker run -p 8080:8080 -v ~/trilium-data:/home/node/trilium-data -e TOTP_ENABLED="true" -e TOTP_SECRET="secret" triliumnext/notes:[VERSION]
  6. Restart Trilium
  7. Go to "Options" -> "MFA"
  8. Click the "Generate Recovery Codes" button
  9. Save the recovery codes. Recovery codes can be used once in place of the TOTP if you loose access to your authenticator. After a rerecovery code is used, it will show the unix timestamp when it was used in the MFA options tab.
  10. Load the secret into an authentication app like google authenticator

OpenID

Currently only compatible with Google. Other services like Authentik and Auth0 are planned on being added.

In order to setup OpenID, you will need to setup a authentication provider. This requires a bit of extra setup. Follow these instructions to setup an OpenID service through google.

Set an environment variable "SSO_ENABLED" to true and add the client ID and secret you obtained from google. Environment variables can be set with a .env file in the root directory, by defining them in the command line, or with a docker container.

.env File

# .env in the project root directory
SSO_ENABLED="true"
BASE_URL="http://localhost:8080"
CLIENT_ID=<client ID from google>
SECRET=<client secret from google>

Environment variable (linux)

export SSO_ENABLED="true"
export BASE_URL="http://localhost:8080"
export CLIENT_ID=<client ID from google>
export SECRET=<client secret from google>

Docker

docker run -d -p 8080:8080 -v ~/trilium-data:/home/node/trilium-data -e SSO_ENABLED="true" -e BASE_URL="http://localhost:8080" -e CLIENT_ID=<client ID from google> -e SECRET=<client secret from google> triliumnext/notes:[VERSION]

After you restart Trilium Notes, you will be redirected to Google's account selection page. Login to an account and Trilium Next will bind to that account, allowing you to login with it.

You can now login using your google account.

@chesspro13 chesspro13 linked an issue Sep 7, 2024 that may be closed by this pull request
@chesspro13 chesspro13 marked this pull request as ready for review September 7, 2024 22:17
@chesspro13 chesspro13 requested review from adoriandoran and a team September 7, 2024 22:18
@perfectra1n
Copy link
Member

This is super cool, thanks for doing this.

Is it also possible to configure the .env variables via the local environment variables? I was poking around in the commits, and didn't see any documentation modified for this change, but I'm assuming that you're saving those changes for once someone else with a much bigger brain than myself reviews it! :)

I'll also review the additional routes for the OTP.

@chesspro13
Copy link
Author

chesspro13 commented Sep 7, 2024

@perfectra1n the environment variables can be set with environment variables (ie export TOTP_ENABLED="true"), -e with docker, and in a .env file in the root directory.

Honestly I forgot to update documentation. Whoops!

edit: I'm working on adding some pages now.

@chesspro13
Copy link
Author

chesspro13 commented Sep 9, 2024

@perfectra1n

Docs complete here.

@chesspro13 chesspro13 requested review from eliandoran and removed request for adoriandoran September 10, 2024 19:21
@chesspro13 chesspro13 changed the title Feature addition: Updated MFA Feature addition: Multi-Factor Authentication Sep 12, 2024
@chesspro13
Copy link
Author

chesspro13 commented Sep 14, 2024

Since you are adding a new table, and it doesn't add it automatically. I got an error here:

@JYC333 I was able to reproduce this and fix it.

No, I mean the database version for the syncing, I think the database version should be the same bewteen client and server. So the previous Trilium (like 0.63.7) won't be compatible. But I see sync version here also, so I'm not sure whether I'm right.

I'll have to look into that. I'm not familiar with the client version.

@perfectra1n When I build docker it is successful, however the setup page doesn't render any text when I tried it again.
Screenshot from 2024-09-14 10-31-18
It's only behaving this was for the Docker image.

Both mobile.ejs and set_passowrd.ejs have some too, but there are none in login.ejs. The only one I made modifications to is login.ejs.

@chesspro13
Copy link
Author

chesspro13 commented Sep 14, 2024

@perfectra1n it looks like the t is part of the i18next package for translations.

const t: TFunction<["translation", ...string[]], undefined>

@perfectra1n
Copy link
Member

Hmm, I wonder why it's throwing an error then, @eliandoran do you have any idea why by chance?

@chesspro13
Copy link
Author

chesspro13 commented Sep 14, 2024

@perfectra1n I can confirm that i18next is what is causing the text to not render. I'm unable to replicate your error. Have you updated all packages with npm i after switching to the new branch?

@perfectra1n
Copy link
Member

perfectra1n commented Sep 14, 2024

image

I can confirm that I did not, but I believe those commands are built within the Docker container without cache -- so you can't replicate the issue when using the Docker container?

@chesspro13
Copy link
Author

chesspro13 commented Sep 14, 2024

so you can't replicate the issue when using the Docker container?

@JYC333 I can't replicate your issue. When I build the docker image it runs, but when I navigate to localhost:8080/setup no text is rendered (the picture I posted above). I think we are both having issues with the same package, but I'm still confused why we are getting different results since we are using docker. I may be wildly wrong, but I'm guessing i18next needs to be installed prior to running the docker build script because the typescript gets compiled in build-docker.sh and copied to the container afterwards in the Dockerfile. My guess is it throws off the translations. ¯\(ツ)

@JYC333
Copy link
Contributor

JYC333 commented Sep 15, 2024

so you can't replicate the issue when using the Docker container?

@JYC333 I can't replicate your issue. When I build the docker image it runs, but when I navigate to localhost:8080/setup no text is rendered (the picture I posted above). I think we are both having issues with the same package, but I'm still confused why we are getting different results since we are using docker. I may be wildly wrong, but I'm guessing i18next needs to be installed prior to running the docker build script because the typescript gets compiled in build-docker.sh and copied to the container afterwards in the Dockerfile. My guess is it throws off the translations. ¯_(ツ)_/¯

Sorry I didn't follow here, is the issue here you for the database issue?

And I didn't try to start with docker, I always run with npm run start-server for development, not sure what's the behaviors when using docker.

@eliandoran
Copy link
Contributor

@perfectra1n I can confirm that i18next is what is causing the text to not render. I'm unable to replicate your error. Have you updated all packages with npm i after switching to the new branch?

There are still some bugs related to internationalization of the server that I'm trying to fix. If it occurs on 'develop' as well it can be ignored for this particular PR.

@perfectra1n
Copy link
Member

@chesspro13 is this still good for testing? :)

I wonder what commits I would have to cherry pick to get the Docker container to build, or how would be best to test it...what have you been doing thus far to test it on your branch?

@chesspro13
Copy link
Author

chesspro13 commented Sep 29, 2024

so you can't replicate the issue when using the Docker container?

@JYC333 I can't replicate your issue. When I build the docker image it runs, but when I navigate to localhost:8080/setup no text is rendered (the picture I posted above). I think we are both having issues with the same package, but I'm still confused why we are getting different results since we are using docker. I may be wildly wrong, but I'm guessing i18next needs to be installed prior to running the docker build script because the typescript gets compiled in build-docker.sh and copied to the container afterwards in the Dockerfile. My guess is it throws off the translations. ¯_(ツ)_/¯

@perfectra1n I tried it in my dev environment as well as on a fresh VM today. The only issue I have is the one with i18next and text not showing up in the setup screen. What OS are you using?

@JYC333 I got the conversations mixed up. That last message I tagged you in was probably supposed to tag @perfectra1n.

Been busy with work related projects and school.

auth0Logout: false,
baseURL: process.env.BASE_URL,
clientID: process.env.CLIENT_ID,
issuerBaseURL: "https://accounts.google.com/.well-known/openid-configuration",
Copy link

@minecraftchest1 minecraftchest1 Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make issuerBaseUrl load process.env.OPENID_CONFIG_URL', and now you have support for more services for OpenID Connect. would also prefix all of the Openid Connect related variables with something like OIDC_' or similar to prevent conflicts.

@@ -69,6 +69,7 @@
"dayjs": "^1.11.13",
"dayjs-plugin-utc": "0.1.2",
"debounce": "^2.1.0",
"dotenv": "^16.4.5",
Copy link

@JeffrinCh JeffrinCh Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chesspro13 Do we need dotenv now? we can use the inbuilt functionality of nodejs now https://nodejs.org/en/learn/command-line/how-to-read-environment-variables-from-nodejs

@eliandoran
Copy link
Contributor

Hi, @chesspro13 .

What's the status of this PR, do you have the time to work on it?

@pano9000
Copy link
Member

quick comment on the env naming:

I personally feel like the env vars should be prefixed with some sort of code, to make sure they are clearly belonging to TriliumNext - which will reduce the chance of accidentally resetting/overwriting these.

E.g. instead of TOTP_ENABLED maybe it could be TRILIUMNEXT_TOTP_ENABLED or just TRILIUM_TOTP_ENABLED

What do you think?

@pano9000
Copy link
Member

quick other remark: since this project seems to use ini files for setting some config options, I would think it makes sense to continue using the existing solution here as well, instead of going to the .env file way.

(Admittedly: yes, it also uses optional env variables, but these are e.g. used to "point" to the data-dir (via TRILIUM_DATA_DIR), where the ini file lies (or gets created, if not existing))

https://github.com/TriliumNext/Notes/blob/develop/config-sample.ini

what do you think?

@chesspro13
Copy link
Author

@eliandoran I think I'm fairly close to closing up my personal project that pulled me away from working on Trilium, so I should be back here sometime this month.

@pano9000 I'll look into implementing it that way. I didn't particularly like using .env, so that seems like a good solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(Feature request) Multi-factor authentication
7 participants